Catering to all possible routes for Post Request by DeepanshuA · Pull Request #700 · dapr/java-sdk

DeepanshuA · 2022-03-09T07:07:45Z

Signed-off-by: deepanshuagarwal deepanshu.agarwal1984@gmail.com

Description

A topic is getting subscribed to in Dapr with a truncated path, in case a class level RequestMapping is also provided with PostMapping path. This change aims at fixing this by taking into account all paths possible for POST.

Issue reference

Please reference the issue this PR will close: #694

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

mukundansundar · 2022-03-14T11:59:51Z

@DeepanshuA Please see to the indentation ... Viewing via GitHub is hard ... please check if the checkstyle passes locally ...

mukundansundar · 2022-03-14T11:59:51Z

@DeepanshuA Please see to the indentation ... Viewing via GitHub is hard ... please check if the checkstyle passes locally ...

mukundansundar

lgtm. @DeepanshuA Can you look into adding Unit Tests for the same?

mukundansundar · 2022-03-16T09:49:06Z

@DeepanshuA
Can you fix DCO ? Also is the PR complete or WIP ?

DeepanshuA · 2022-03-16T10:47:55Z

@DeepanshuA Please see to the indentation ... Viewing via GitHub is hard ... please check if the checkstyle passes locally ...

yeah @mukundansundar, fixed indentation

DeepanshuA · 2022-03-16T10:48:17Z

@DeepanshuA Please see to the indentation ... Viewing via GitHub is hard ... please check if the checkstyle passes locally ...

fixed indentation

DeepanshuA · 2022-03-16T10:48:45Z

lgtm. @DeepanshuA Can you look into adding Unit Tests for the same?

yups, added, was in WIP actually before this

DeepanshuA · 2022-03-16T10:50:01Z

@DeepanshuA Can you fix DCO ? Also is the PR complete or WIP ?

yeah @mukundansundar, missed out on sign-off part, let me fix it; yeah, will change the title - it will be PR complete after DCO change.

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

mukundansundar

small change

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

mukundansundar · 2022-03-16T17:35:15Z

cc @artursouza

artursouza · 2022-03-16T23:50:58Z

+   * @param topicName Associated topic name
+   * @return All possible routes for post mapping for this class and post method
+   */
+  public static List<String> getAllCompleteRoutesForPost(Class clazz, Method method, String topicName) {


Please, move this as a private method in DaprBeanPostProcessor. The reason is that this design will make this method public and part of our contract - in other words, users can depend on this method. Marking it as private makes it clear that is internal use only. Unfortunately, we don't have an internal visibility, only protected, public, private and default.

Agreed that this will expose it without need of exposing. Though, just thinking that at least taking out some methods with default access (package private)/ protected may be required, if refactoring a huge class OR using a required design pattern may force us making some extra methods/interfaces public sometimes. BUT, that's not the case with DaprBeanPostProcessor as of now. So, making the suggested change.

artursouza · 2022-03-16T23:51:39Z

+
+  @Test
+  public void testAllPostRoutesGeneration() throws NoSuchMethodException {
+    List<String> routesArrayTestMethod1 = SpringProcessorHelper.getAllCompleteRoutesForPost(clazzToBeTested,


After this method is made private, you can still test it via reflection.

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

mukundansundar

lgtm

codecov · 2022-03-21T08:37:05Z

Codecov Report

Merging #700 (cee4dd2) into master (06d92da) will decrease coverage by 1.52%.
The diff coverage is 68.42%.

@@             Coverage Diff              @@
##             master     #700      +/-   ##
============================================
- Coverage     79.62%   78.10%   -1.53%     
- Complexity     1092     1106      +14     
============================================
  Files            92       97       +5     
  Lines          3308     3407      +99     
  Branches        376      394      +18     
============================================
+ Hits           2634     2661      +27     
- Misses          482      547      +65     
- Partials        192      199       +7

Impacted Files	Coverage Δ
...java/io/dapr/springboot/DaprBeanPostProcessor.java	`42.18% <68.42%> (ø)`
...java/io/dapr/springboot/DaprTopicSubscription.java	`0.00% <0.00%> (ø)`
.../src/main/java/io/dapr/springboot/DaprRuntime.java	`0.00% <0.00%> (ø)`
...java/io/dapr/springboot/DaprAutoConfiguration.java	`0.00% <0.00%> (ø)`
...c/main/java/io/dapr/springboot/DaprController.java	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06d92da...cee4dd2. Read the comment docs.

* Catering to all possible routes for Post Request Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Correcting checkstyle related violations Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Refactoring and Adding UTs for post method routes Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Using resolved topic name Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Shifting routesPostMethod to reduce its access Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com> Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>

* Catering to all possible routes for Post Request Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Correcting checkstyle related violations Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Refactoring and Adding UTs for post method routes Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Using resolved topic name Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> * Shifting routesPostMethod to reduce its access Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com> Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>

DeepanshuA requested review from a team as code owners March 9, 2022 07:07

mukundansundar reviewed Mar 14, 2022

View reviewed changes

DeepanshuA force-pushed the issue694 branch from e9a2df0 to 2c143d4 Compare March 16, 2022 11:43

DeepanshuA added 3 commits March 16, 2022 17:40

Catering to all possible routes for Post Request

1592c2b

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

Correcting checkstyle related violations

a2edb21

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

Refactoring and Adding UTs for post method routes

70435c5

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

DeepanshuA force-pushed the issue694 branch from 2c143d4 to 70435c5 Compare March 16, 2022 12:14

DeepanshuA changed the title ~~[WIP] Catering to all possible routes for Post Request~~ Catering to all possible routes for Post Request Mar 16, 2022

DeepanshuA requested review from mukundansundar and removed request for a team March 16, 2022 12:54

mukundansundar suggested changes Mar 16, 2022

View reviewed changes

Comment thread sdk-springboot/src/main/java/io/dapr/springboot/DaprBeanPostProcessor.java Outdated

Using resolved topic name

2455756

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

DeepanshuA requested a review from mukundansundar March 16, 2022 17:32

mukundansundar previously approved these changes Mar 16, 2022

View reviewed changes

artursouza suggested changes Mar 16, 2022

View reviewed changes

Shifting routesPostMethod to reduce its access

76f49eb

Signed-off-by: deepanshuagarwal <deepanshu.agarwal1984@gmail.com>

DeepanshuA dismissed mukundansundar’s stale review via 76f49eb March 17, 2022 15:44

mukundansundar requested a review from artursouza March 17, 2022 16:14

mukundansundar approved these changes Mar 17, 2022

View reviewed changes

Merge branch 'master' into issue694

cee4dd2

artursouza approved these changes Mar 21, 2022

View reviewed changes

artursouza added the auto-merge label Mar 21, 2022

mukundansundar merged commit 593da48 into dapr:master Mar 21, 2022

Uh oh!

Conversation

DeepanshuA commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

mukundansundar commented Mar 14, 2022

Uh oh!

mukundansundar commented Mar 14, 2022

Uh oh!

mukundansundar left a comment

Choose a reason for hiding this comment

Uh oh!

mukundansundar commented Mar 16, 2022

Uh oh!

DeepanshuA commented Mar 16, 2022

Uh oh!

DeepanshuA commented Mar 16, 2022

Uh oh!

DeepanshuA commented Mar 16, 2022

Uh oh!

DeepanshuA commented Mar 16, 2022

Uh oh!

mukundansundar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mukundansundar commented Mar 16, 2022

Uh oh!

artursouza Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

DeepanshuA Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

artursouza Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

DeepanshuA Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

mukundansundar left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 21, 2022

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DeepanshuA commented Mar 9, 2022 •

edited

Loading